Add Keccak-256 (Ethereum-style, original 0x01 padding)#3245
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3245 +/- ##
==========================================
- Coverage 78.42% 78.40% -0.03%
==========================================
Files 695 695
Lines 124087 124144 +57
Branches 17226 17236 +10
==========================================
+ Hits 97321 97334 +13
- Misses 25845 25886 +41
- Partials 921 924 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Really clean PR! My only major comment is around the decision to add Keccak256 into the FIPS module. Would it be possible to integrate it outside of the module instead, similar to how blake2 is handled, while reusing the FIPS 202 functions from the FIPS module? This would also mean relocating the test vectors.
| 0x85, 0x3c, 0x64, 0xa1, 0x56, 0x6f, 0xeb, 0x76, 0x25, 0x9a, 0x4a, 0x44, | ||
| 0x23, 0xf7, 0xcf, 0x46}; | ||
|
|
||
| // Keccak-256 of |kPlaintext|, computed by util/keccak_ref.py. |
There was a problem hiding this comment.
util/gen_keccak256_kat.py ?
There was a problem hiding this comment.
fixed in second commit
| } | ||
|
|
||
| // |EVP_MD_FLAG_DIGALGID_ABSENT| matches OpenSSL 3.2+'s | ||
| // |PROV_DIGEST_FLAG_ALGID_ABSENT| on KECCAK-{224,256,384,512}. With |
There was a problem hiding this comment.
Good point. Are there other algorithms that should update the flag EVP_MD_FLAG_DIGALGID_ABSENT?
| // Keccak-256 has no NID/OID (the Ethereum-style 0x01-padding variant is | ||
| // not standardised). Registered by name only; matches OpenSSL 3.2+'s | ||
| // "KECCAK-256" provider name for cross-library lookups. | ||
| {NID_undef, EVP_keccak_256, "KECCAK-256", "keccak-256"}, |
There was a problem hiding this comment.
Do we need the keccak-256 entry? blake2 has no entry in the table. Should we add it as well?
There was a problem hiding this comment.
We need this entry to be able to use EVP_get_digestbyname("KECCAK-256"), which is necessary to make it compatible with OpenSSL 3. OpenSSL 3 does not support EVP_keccak_256().
| // permutation as the SHA-3 family but is NOT FIPS-approved: it does not | ||
| // update the FIPS service indicator. Streaming functions are exposed only | ||
| // internally; public consumers reach Keccak-256 via |EVP_keccak_256|. | ||
| // |
There was a problem hiding this comment.
nit: extra comment line here
There was a problem hiding this comment.
fixed in second commit
| !SpeedHash(EVP_sha3_384(), "SHA3-384", selected) || | ||
| !SpeedHash(EVP_sha3_512(), "SHA3-512", selected) || | ||
| #endif | ||
| #if defined(OPENSSL_IS_AWSLC) |
There was a problem hiding this comment.
Is it not compatible with openssl?
There was a problem hiding this comment.
OpenSSL 3 does not support EVP_keccak_256() which this code uses.
| @@ -0,0 +1,105 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Missing license header.
There was a problem hiding this comment.
fixed in second commit
| out_dir = os.path.normpath(out_dir) | ||
| os.makedirs(out_dir, exist_ok=True) | ||
|
|
||
| # Sanity check pycryptodome's output against the well-known Ethereum |
| crypto/fipsmodule/sha/testvectors/SHA3_384ShortMsg.txt | ||
| crypto/fipsmodule/sha/testvectors/SHA3_512LongMsg.txt | ||
| crypto/fipsmodule/sha/testvectors/SHA3_512ShortMsg.txt | ||
| crypto/keccak/testvectors/KECCAK_256ShortMsg.txt |
There was a problem hiding this comment.
Alphabetical order would be better
There was a problem hiding this comment.
fixed in second commit
- Add missing license header to util/gen_keccak256_kat.py - Define SN_keccak_256/LN_keccak_256 in nid.h and use them in nid_to_digest_mapping instead of raw string literals - Fix incorrect script name in service_indicator_test.cc comment - Remove extra blank comment line in internal.h - Fix alphabetical ordering of keccak test vectors in sources.cmake
|
Thank you for the detailed review @manastasova! I've added a new commit to address your comments. Re:
We would still need to make 2 changes to the FIPS module:
Let me know if you prefer doing it that way. |
- Add missing license header to util/gen_keccak256_kat.py - Define SN_keccak_256/LN_keccak_256 in nid.h and use them in nid_to_digest_mapping instead of raw string literals - Fix incorrect script name in service_indicator_test.cc comment - Remove extra blank comment line in internal.h - Fix alphabetical ordering of keccak test vectors in sources.cmake
- Add missing license header to util/gen_keccak256_kat.py - Define SN_keccak_256/LN_keccak_256 in nid.h and use them in nid_to_digest_mapping instead of raw string literals - Fix incorrect script name in service_indicator_test.cc comment - Remove extra blank comment line in internal.h - Fix alphabetical ordering of keccak test vectors in sources.cmake
|
Re:
These changes sound reasonable. |
Implement the pre-standardization Keccak-256 hash, the variant that uses
the original 0x01 padding byte as used by Ethereum and Solidity's
keccak256(). It shares the rate, capacity and digest length of FIPS 202
SHA3-256 but differs in the padding byte (0x06), so it produces distinct
digests and is not FIPS-approved.
The implementation lives outside the FIPS module in crypto/keccak/,
following the blake2 template:
- crypto/keccak/keccak.c + internal.h provide Keccak256_Init/Update/
Final and the Keccak256 one-shot.
- The EVP_MD wrapper EVP_keccak256() lives in digest_extra.c alongside
the other non-FIPS digests (EVP_md4, EVP_blake2b256).
- It is registered by name only as "KECCAK-256" (plus the "keccak-256"
lowercase alias) with NID_undef, matching OpenSSL 3.2+'s provider
name so EVP_get_digestbyname() lookups are cross-compatible. Because
Keccak-256 has no OID/NID, the names are string literals in the
digest table rather than SN_/LN_ macros (nid.h is generated from
objects.txt and cannot carry an OID-less entry). It has no OID and
must not be used where a digest AlgorithmIdentifier is marshalled.
- Tests are in crypto/keccak/keccak256_test.cc; digest_extra keeps only
a generic vector row and the getter assertions, as blake2 does.
Changes to the FIPS module are limited to giving the shared FIPS 202
buffering primitives (FIPS202_Reset/Update/Finalize) external linkage so
crypto/keccak can reuse the audited Keccak-f[1600] permutation and
padding logic instead of duplicating it. FIPS202_Init is unchanged and
still rejects non-FIPS-202 padding; Keccak256_Init sets up the context
with the 0x01 padding byte itself.
AWSLC_API_VERSION is bumped to 36 so consumers can feature-detect
EVP_keccak256 (v5.2.0 already shipped version 35 without it); the
bssl speed benchmark gates its EVP_keccak256 call on AWSLC_API_VERSION
>= 36 accordingly.
The KAT vectors (KECCAK256ShortMsg.txt / KECCAK256LongMsg.txt) are
generated by util/gen_keccak256_kat.py against pycryptodome. The embedded
test data in generated-src/crypto_test_data.cc.tar.bz2 was regenerated
with util/generate_build_files.py, and the version scripts
(crypto/libcrypto.txt/.map) with util/generate_initial_version_scripts.sh.
EVP_keccak256 is exported at the AWS_LC_1.0 node.
|
Pushed the refactor implementing the approved direction (move outside the FIPS module, mirror blake2). Summary of what changed since the last review: Moved Keccak-256 out of the FIPS module — it now lives in a new The two FIPS-module changes we agreed on:
Public API renamed Tests moved to a dedicated Bumped Name registration: since Keccak-256 has no OID/NID, |
Issues:
N/A
Description of changes:
[edit] Following review, Keccak-256 was moved out of the FIPS module into
crypto/keccak/(mirroring blake2), reusing the module's FIPS 202 primitives via external linkage;FIPS202_Initis unchanged. The public API is nowEVP_keccak256()(no underscore), tests live incrypto/keccak/keccak256_test.cc, andAWSLC_API_VERSIONis bumped to 36. See the comment below for the full changelog — the "Approach" section above describes the original in-module design and is superseded.Adds Keccak-256 with the original Keccak submission's padding byte (0x01) — the variant used by Ethereum, Solidity's
keccak256(), and other blockchain ecosystems. This is not FIPS 202 SHA3-256 (which uses 0x06 padding); rate (1088 bits) and digest length (256 bits) match, but the padding byte differs, so outputs differ for every input.Approach:
The implementation lives inside the FIPS module
(
crypto/fipsmodule/sha/sha3.c) so it shares the audited Keccak-f[1600] permutation, FIPS202_Init/Update/Finalize buffering, and assembly backends with the SHA-3 family. The only delta in the module's algorithmic code path is one extra accepted padding byte:The new public surface is exactly
EVP_keccak_256()(plus the existingEVP_Digest*machinery). This matches the BoringSSL/AWS-LC convention established by SHA-3: theEVP_MDis the public API; direct C entry points (KECCAK_256_Init/Update/Final, one-shotKECCAK_256()) live incrypto/fipsmodule/sha/internal.hand are exported only for in-tree consumers, mirroringSHA3_256_*.Call-outs:
NOT FIPS-approved.
KECCAK_256_Finaland theKECCAK_256()one-shot intentionally omitFIPS_service_indicator_update_state(), so calling Keccak-256 leaves the per-thread approved-services counter unchanged. Verified by a new row inservice_indicator_test.cc::kDigestTestVectorswithAWSLC_NOT_APPROVED. The negative invariant is enforced both at runtime (the parameterizedEVPMDServiceIndicatorTest.EVP_Digestscatches accidental_update_statecalls) and structurally (a future reviewer can grep the file and see only "Intentionally no" comments).No NID, no OID. The Ethereum-style 0x01-padding Keccak is not standardised and OpenSSL has not assigned an OID for it either. The EVP_MD's
typeisNID_undef, and the digest is registered innid_to_digest_mappingby name only. As a consequence,EVP_keccak_256()cannot be used withEVP_DigestSign*/EVP_DigestVerify*for RSA-PKCS#1-v1_5 or ECDSA, andEVP_marshal_digest_algorithmwill fail withDIGEST_R_UNKNOWN_HASH.objects.txt,obj_dat.h, andnid.hare intentionally untouched.OpenSSL compatibility. The registered EVP_MD name is
KECCAK-256(uppercase, hyphenated), matching OpenSSL 3.2+'s default-provider registration inproviders/implementations/include/prov/names.h. Portable code that doesEVP_MD_fetch(NULL, "KECCAK-256", NULL)on OpenSSL works against AWS-LC viaEVP_get_digestbyname("KECCAK-256"). Akeccak-256lowercase alias is also registered.FIPS module impact. The audited Keccak-f[1600] permutation, absorb/squeeze code, and existing SHA-3 entry points are unchanged byte-for-byte. The only edit inside the module boundary is the 3-line
FIPS202_Initpad-byte check above plus the newKECCAK_256_*wrappers.Testing:
All Keccak-256 tests live in
crypto/digest_extra/digest_test.ccalongside the rest of the digest tests. New coverage:kTestVectors[]— 3 well-known Ethereum-ecosystem vectors (empty / "abc" / fox-pangram). These exercise the EVP one-shot, byte-at-a-time streaming, copy/move-on-context, and unaligned-input paths inTestDigest().DigestTest.Keccak256DiffersFromSHA3_256— invariant: for the same input, Keccak-256 and SHA3-256 outputs must always differ. Guards against a future maintainer "fixing" the padding byte.DigestTest.Keccak256KAT— file-driven tests using 137 short-message vectors (0..1088 bits) and 100 long-message vectors (2184..110688 bits), in the sameLen/Msg/MDformat used by SHA-3 KATs. Mirrors theFileTestGTestpattern fromsha3_test.cc::NISTTestVectors.DigestTest.Getters— extended withEVP_get_digestbyname("KECCAK-256"), the lowercase alias, and anEVP_MD_type == NID_undefassertion.service_indicator_test.cc::kDigestTestVectors— one new row, parameterized asEVP_Digests/13, assertsAWSLC_NOT_APPROVEDthrough Init / Update / Final.KAT vectors are generated by
util/gen_keccak256_kat.py, which uses pycryptodome (an independent Keccak-256 implementation) as the reference. The script sanity-checks pycryptodome against the well-known Ethereum vectors before generating, so a misconfigured environment fails loudly. Reference and implementation under test come from completely different code, so the test asserts genuine cross-implementation agreement.bssl speed -filter KECCAK-256is wired up next to the SHA-3 benchmarks for performance regression tracking.Verified end-to-end:
crypto_testcases pass, includingDigestTest.Keccak256*,SHA3Test.*(no regression), and the full digest suite.-DFIPS=1 -DBUILD_SHARED_LIBS=1): 3719/3719 active cases pass, including all 14 rows ofEVPMDServiceIndicatorTest.EVP_Digests(was 13).bssl speed -filter KECCAK-256produces sensible numbers across all default chunk sizes.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.